New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upgrade request handling: ignore http/2 and optionally ignore websocket #1661
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we reach handle_upgrade
, we know that upgrade_value == b"websocket"
, so we can remove the header verification over there.
uvicorn/protocols/http/h11_impl.py
Outdated
if name == b"connection": | ||
connection = [token.lower().strip() for token in value.split(b",")] | ||
if name == b"upgrade": | ||
upgrade = value.lower().strip() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for stripping?
upgrade = value.lower().strip() | |
upgrade = value.lower() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No specific reason, kept it for symmetry reasons and because I generally like to sanitize inputs. Removed it
You're right, I kept it in if we want to support additional protocols, but that's a bad reason |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we should rename the handle_upgrade
to handle_websocket_upgrade
, but that's not a blocker.
Those two last comments were my last here. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hold on... I want the event parameter out. 👀
Sry the previous commit only changed the signature not the call, have been doing multiple things at once.. |
wont have the time sorry |
Ok. I'll deal with this. Thanks for letting me know. 🙏 |
@Argannor I actually have a question that I didn't think before... But if I have one of the |
@Kludex The application might run in a shared environment (globally installed packages) but not implement websocket endpoints. |
But you can set the |
uvicorn/protocols/http/h11_impl.py
Outdated
return ( | ||
b"upgrade" in connection | ||
and upgrade == b"websocket" | ||
and not self.config.ws_ignore_upgrade |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm I think we could eliminate the new flag by changing it to this
and not self.config.ws_ignore_upgrade | |
and self.ws_protocol_class is not None |
and removing the corresponding block from handle_websocket_upgrade
. I think that would achieve the same thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the warning should stay tho 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So trying to understand what that block tries to achieve is
- logging a warning
- logging another warning if no protocol was found
- responding with 400
But should these warnings really be logged on every upgrade request again? Especially the second one.
In my use case this will lead to a lot of unnecessary log messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about moving the second log statement to the run method in main.py? I feel like this indicates a possible configuration / setup issue which would be easier to catch if printed up front on startup. We should rephrase to account for the fact that there are applications which will use the default setting auto
without the need for an implementation to be present.
I am still undecided on what to do with the first warning. Since I already have a custom log handler, I could filter this message out to prevent the broken window for me.
Alternatively we could document the behaviour and necessary remediation inside the docs to help confused devs figure out why their request gets ignored and or turn down the log level.
Side note: I am not available today, but will come back to this tomorrow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi folks.
Thanks for looping me into the review @Kludex. There's two different things being addressed here, and I think we need to treat them independently...
- Ignore
Upgrade
requests that we don't handle. As raised by Gracefully handle HTTP/2 upgrade request #1501. - Support websockets being configured as "off" without raising warnings on ws upgrades. As discussed in Ignore Websocket Upgrade Requests #1659.
I would suggest that we start off by dealing with the first of these. That looks clearly sensible to me, and we can improve our behaviour here without adding any additional configuration.
It looks to me like the second of these cases should be handled by ensuring that --ws none
has the desired behaviour, rather than adding an additional configuration option? (As it happens that would probably be a much better default than auto
, but that's a different consideration.)
… availability of a ws implementation.
Hi @tomchristie thanks for having a look at this too! I pushed a new iteration of this PR and integrated the feedback of both of you. The check is now based on the I also tried to split the concerns of #1501 and #1659, however I think that it makes the code more complicated in the case of httptools. While still trying to use the natively implemented |
This seems sensible to me now. |
return upgrade | ||
return None | ||
|
||
def _should_upgrade_to_ws(self) -> bool: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the one in the other protocol is implemented in one way, and this one in another? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both protocols work a bit different, and in the case of httptools the "check and return" is repeated in several places, but the actual upgrade handling is only done once in the except block. While the h11 implementation only does it in one single place and both the returning and handling is in the same place.
We are checking all the headers 3 times on the |
I'm checking if it's a problem to iterate over the headers multiple times for httptools
Hi @Kludex, thank you for looking into this. Are there any updates, or changes that I should make? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Argannor 🙏
This PR addresses the discussion #1659 (raised by me) and issue #1501.
Rationale: As outlined in the discussion and issue above the HTTP/1.x spec allows servers to ignore upgrade requests by responding with a normal response.
Changes made:
ws_ignore_upgrades
toTrue
_should_upgrade
in both http protocol implementations to be overwritten by users that need a more fine grained logic to decide on upgradesAfter making the change for #1659 I also stumbled across the PR #1642 (adressing #1501). Since both changes affect the same area I took the liberty to refactor my change to include graceful handling of #1501 as well.
Looking forward to your feedback!